Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(changed-files): limit git and hg commands to specified roots #6732

Merged
merged 5 commits into from
Jul 21, 2018
Merged

perf(changed-files): limit git and hg commands to specified roots #6732

merged 5 commits into from
Jul 21, 2018

Conversation

booleanbetrayal
Copy link
Contributor

Summary

Given a monorepo setup under git version control, we saw jest --watch commands take significantly longer in a Docker container host mount (rw:cached) than when on the host directly. Tracked the issue down to the fact that git and hg command used to determine lastCommit changes were traversing the entire repository, and not just paths specified in the Jest config roots values. Given the mounted container latency, things like basic lstat gathering be intensive in a large repository. The roots we had specified are rather shallow themselves and only contain things we want to test with Jest. When limiting the scope of the git command to just the roots paths, we saw change determination drop from 2.5m to 3s in our container.

This implementation seem to align more closely with the roots documentation which indicates:

A list of paths to directories that Jest should use to search for files in.

It did seem a little unexpected that Jest would be traversing our entire repo.

Test plan

Specs have been added for new functionality a build from this fork is currently being used in multiple environments.

For reference, here is our Jest configuration:

"jest": {
    "rootDir": "spec/puppeteer",
    "testPathIgnorePatterns": [
        "__image_snapshots__"
    ],
    "testMatch": [
        "<rootDir>/*_screenshot_spec.js"
    ],
    "reporters": [
        "default",
        [
            "jest-junit",
            {
                "output": "test-results/puppeteer.xml"
            }
        ]
    ],
    "verbose": true
}

The spec/puppeteer subdirectory contains only a handful of files whereas the project containing the package.json itself is quite expansive. A similar environment staging is probably easily recreated.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find! Let's just remember it's a breaking change for jest-changed-files.

@@ -46,33 +46,44 @@ const findChangedFilesUsingCommand = async (
const adapter: SCMAdapter = {
findChangedFiles: async (
cwd: string,
roots: Array<Path>,
options?: Options,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about moving roots to Options? This way we could keep it backwards compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the idea that we would keep roots as optional within Options but ensure that getChangedFilesForRoots always provides it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, ideally with the default behavior being the same as current one (pre this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed the fix. Let me know what you think. Thanks!

@booleanbetrayal
Copy link
Contributor Author

@thymikee - Just rebased that last commit with some tweaks. Let me know if that works for your migration path.

@codecov-io
Copy link

codecov-io commented Jul 20, 2018

Codecov Report

Merging #6732 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6732      +/-   ##
==========================================
- Coverage   63.68%   63.68%   -0.01%     
==========================================
  Files         235      235              
  Lines        9003     9007       +4     
  Branches        4        4              
==========================================
+ Hits         5734     5736       +2     
- Misses       3268     3270       +2     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-changed-files/src/hg.js 28.57% <0%> (-1.74%) ⬇️
packages/jest-changed-files/src/git.js 94.59% <100%> (+0.15%) ⬆️
packages/jest-changed-files/src/index.js 83.33% <100%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf9cbc2...3647446. Read the comment docs.

@thymikee thymikee requested review from aaronabramov and SimenB July 20, 2018 21:43
Copy link
Contributor

@aaronabramov aaronabramov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow.. this can actually be a huge win for facebook repos

@@ -326,3 +347,31 @@ test('gets changed files for hg', async () => {
.sort(),
).toEqual(['file5.txt']);
});

test('should only monitor root paths for hg', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should avoid using should in test titles :)
use it('monitors only root paths for hg'...


run(`${GIT} init`, DIR);

const roots = ['nested_dir', 'nested_dir/second_nested_dir'].map(filename =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like nested_dir already includes nested_dir/second_nested_dir root

Copy link
Contributor Author

@booleanbetrayal booleanbetrayal Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated / removed redundant root!

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, thanks!

@booleanbetrayal
Copy link
Contributor Author

Sounds great! Let me know if there's anything you all would like me to do prior to merge (squash rebase or any tweaks?). Otherwise, when might it make it to master?

@thymikee
Copy link
Collaborator

@SimenB you ok with changelog? I'm keen on merging this and let @aaronabramov test under some heavy load :)

@SimenB
Copy link
Member

SimenB commented Jul 21, 2018

Go wild 😀

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants